Add deprecation warnings to BidsDataset properties slated for change
#251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently using a custom fork of the library to get allow custom deprecation admonitions in the docs
tkkuehn
approved these changes
Feb 21, 2023
|
@tkkuehn, great, so no concern about |
|
Yeah, my sense is that it's more important for end users not to get drowned in a bunch of error messages they can't address than for maintainers to see these warnings. I think updating the docs should be sufficient for anyone who's likely to eventually make the switch to Snakebids 1.0.0 (and we'll point them to the changes increasingly more clearly as we approach release of 1.0.0). |
kaitj
approved these changes
Feb 21, 2023
|
Great, just a few text changes I noticed that I'd like to fix before merging |
Deprecate the Component getter properties (that currently allow dataset->attr->component) to allow for future modification of those properties
The entire docstring can still be copied using the copy_extended_docstring property
Remove existing references to the deprecated BidsDataset properties. Some functions, including the tests of these properties, call them by necessity, so DeprecationWarnings are ignored at these points
The test was checking if dataset.input_lists was equal to itself, rather than to dataset.entities
pvandyken
force-pushed
the
deprecate-bidsdataset-features
branch
from
c47586f to
402d3a4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #237.
This PR does not yet include any changes to the docs, as the rewrite will be a bit complicated, and I may want to incorporate the pretty printing #197 into it to ease the explanations.
I'm using the deprecation library, or rather, a custom fork of it. I wanted the deprecations to be displayed prominently in the docs, and the default sphinx directive just displays it as vanilla text without any box or anything. So I modified deprecation to allow me to nest the deprecation message in a warning box. I've raised an issue there about incorporating this into the main repository.
One other note: by default, Python actually doesn't display
DeprecationWarnings. This is to prevent end-users of an app from seeing warnings that don't pertain to them. For example, if Hippunfold upgradessnakebidsto this PR but doesn't make the syntax changes in their workflow, there's no reason for their users to see all the resulting warning messages.In order to the warnings they have to be enabled. Test suites do this by default. You can also call your script as
python -Wdefault.The problem is I don't know that snakemake users are typically using test suites on their workflows, and would probably never think to use the
-Wflag. So probably no one will ever see the warnings.So we can just rely on the docs and changelogs to get people to migrate. There's also a separate
FutureWarningthat is always displayed, but then end users of the app will also see it.Any ideas or thoughts on this are appreciated.